Refacto ack and read #8100
Refacto ack and read #8100
Conversation
📝 WalkthroughWalkthroughThis pull request shifts the responsibility for handling card read and acknowledgment actions from the existing service to a newly introduced service, Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Signed-off-by: freddidierRTE <[email protected]>
freddidierRTE
force-pushed
the
FE-refactoAck
branch
from
050ff3e to
e19524f
Compare
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
services/cards-publication/src/main/java/org/opfab/cards/publication/services/CardReadAndAckService.java (2)
26-28: Initialize fields as final for immutability.
If these service references are not intended to be re-assigned, consider declaring them as final to ensure immutability and improve clarity.- private CardNotificationService cardNotificationService; - private CardRepository cardRepository; - private CardPermissionControlService cardPermissionControlService; + private final CardNotificationService cardNotificationService; + private final CardRepository cardRepository; + private final CardPermissionControlService cardPermissionControlService;
39-49: Refactor membership check logic to avoid duplication and improve clarity.
You implement a membership check here and again at lines 73–78. Consider extracting this logic into a dedicated helper method to keep the code DRY.+ private void validateUserEntities(CurrentUserWithPerimeters user, List<String> entitiesAcks, String errorMsg) { + if (!user.getUserData().getEntities().containsAll(entitiesAcks)) { + throw new ApiErrorException(new ApiError(HttpStatus.FORBIDDEN, errorMsg)); + } + } public UserBasedOperationResult processUserAcknowledgement(String cardUid, CurrentUserWithPerimeters user, List<String> entitiesAcks) { if (cardPermissionControlService.isCurrentUserReadOnly(user) && entitiesAcks != null && !entitiesAcks.isEmpty()) { throw new ApiErrorException( new ApiError(HttpStatus.FORBIDDEN, "Acknowledgement impossible : User has READONLY opfab role")); } - if (!user.getUserData().getEntities().containsAll(entitiesAcks)) - throw new ApiErrorException( - new ApiError(HttpStatus.FORBIDDEN, - "Acknowledgement impossible : User is not member of all the entities given in the request")); + validateUserEntities(user, entitiesAcks, + "Acknowledgement impossible : User is not member of all the entities given in the request"); cardRepository.findByUid(cardUid).ifPresent(selectedCard -> cardNotificationService.pushAckOfCardInEventBus(cardUid, selectedCard.id, entitiesAcks, CardOperationTypeEnum.ACK)); ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
services/cards-publication/src/main/java/org/opfab/cards/publication/configuration/Services.java(5 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/controllers/CardController.java(8 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/services/CardProcessingService.java(0 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/services/CardReadAndAckService.java(1 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java(3 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardReadAndAckServiceShould.java(1 hunks)
💤 Files with no reviewable changes (1)
- services/cards-publication/src/main/java/org/opfab/cards/publication/services/CardProcessingService.java
🔇 Additional comments (24)
services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java (4)
83-83: New CardReadAndAckService field added to test classThis field has been added as part of the refactoring to extract card read and acknowledgment functionality into a dedicated service.
114-114: Properly initializing the new CardReadAndAckServiceThe service is correctly initialized with the necessary dependencies (cardNotificationService and cardRepositoryMock).
892-894: Updated to use new CardReadAndAckService for user readsThe test now uses the dedicated CardReadAndAckService instead of the CardProcessingService for handling user reads, which aligns with the architectural refactoring.
897-898: Updated to use new CardReadAndAckService for user acknowledgementsThe test correctly uses the new dedicated service for processing acknowledgements, maintaining the same test behavior while adapting to the new architecture.
services/cards-publication/src/main/java/org/opfab/cards/publication/configuration/Services.java (5)
22-22: Added import for the new CardReadAndAckServiceThe import is correctly added to make the new service class available.
42-43: Added CardReadAndAckService as a private fieldThis field declaration follows the same pattern as other services in this class, maintaining consistency.
71-72: Improved formatting of CardNotificationService initializationThe service initialization has been reformatted for better readability.
83-83: Initializing the new CardReadAndAckServiceThe service is properly initialized with its required dependencies.
99-101: Added getter method for CardReadAndAckServiceThis getter method follows the same pattern as other service getters in this class, maintaining consistency in the API.
services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardReadAndAckServiceShould.java (4)
25-27: New test class with proper Spring annotationsThe test class is correctly set up with Spring test annotations to ensure proper integration with the test context.
32-37: Well-defined service and dependency fieldsThe test properly defines fields for the service under test and its dependencies, following standard testing patterns.
39-48: Clear test setup with BeforeEachThe initialization method properly sets up the testing environment before each test by creating fresh instances of dependencies and clearing the repository and event spy.
50-56: Test verifies event bus notification on reset operationThis test ensures that when card acknowledgments and reads are reset, an appropriate UPDATE notification is sent to the event bus, which is important for the system's reactive behavior.
services/cards-publication/src/main/java/org/opfab/cards/publication/controllers/CardController.java (8)
22-22: Added import for the new CardReadAndAckServiceThe import statement is correctly added to make the new service available in the controller.
48-48: Added CardReadAndAckService as a private fieldThe new service field is properly added to the controller class.
59-59: Initializing CardReadAndAckService in the constructorThe service is correctly retrieved from the Services class in the constructor.
180-180: Updated to use CardReadAndAckService for user acknowledgementsThe controller now delegates acknowledgement processing to the specialized service instead of the generic card processing service.
208-208: Updated to use CardReadAndAckService for card read operationsProperly redirects the card read API call to the dedicated service.
234-234: Updated to use CardReadAndAckService for acknowledgement deletionThe controller now uses the specialized service for managing acknowledgement deletion.
259-259: Updated to use CardReadAndAckService for read deletionThe controller correctly uses the dedicated service for handling read status deletion.
309-309: Updated to use CardReadAndAckService for resetting reads and acknowledgementsThe reset operation is now properly delegated to the specialized service.
services/cards-publication/src/main/java/org/opfab/cards/publication/services/CardReadAndAckService.java (3)
50-56: Confirm that the code handles missing cards as intended.
If the card cannot be found by UID, no event is triggered, and the method still returns a result. Ensure this silent behavior is what you want or consider informing the caller that the card does not exist.
73-78: Duplicate membership check logic.
This block duplicates the logic in lines 39–49.
86-95: Resetting read and ack states is consistent with the new service approach.
This logic effectively notifies the event bus only if the card is found, aligning well with the new separation of concerns.
Back : extract ack an read logic in a new service